-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Init MLflow support #2
Conversation
b551cdb
to
16c3c80
Compare
) | ||
|
||
def _generate_mlflow_root_task_sql(self): | ||
return """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-use self.TASK_TEMPLATE
?
sql_statements = [self._generate_root_task_sql()] | ||
sql_statements = [ | ||
self._generate_root_task_sql(), | ||
self._generate_root_task_suspend_sql(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be immediately execute suspend
after execute
? Will it work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not ? For a brand new pipeline it is always suspended - so does not take any effect - for a replaced one it should just change the state ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe it would make sense to always drop a whole pipeline and then recreate it from scratch - it would then fix #3 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be, let's discuss it.
For the suspend
vs execute
- wouldn't the pipeline stop executing, because of the "suspend"?
4af2040
to
7435c6e
Compare
fc6f13d
to
83dcd45
Compare
7a4c2d1
to
6daf8d2
Compare
ea85f61
to
5d64d40
Compare
e321728
to
acccc84
Compare
1c59887
to
7dc2061
Compare
## Deployment to Snowflake and inference | ||
|
||
### Deployment | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just add link to https://github.com/Snowflake-Labs/mlflow-snowflake ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
log_parameter(k, v) | ||
|
||
|
||
def log_model(model: RandomForestRegressor, X_train: pd.DataFrame, y_train: pd.Series): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover?
... cookiecutter.repo_name }}/src/{{ cookiecutter.python_package }}/pipelines/mlflow_helpers.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok to merge
SonarCloud Quality Gate failed. 0 Bugs 65.7% Coverage Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
No description provided.